Create new _sdl3_mixer module#3692
Open
Starbuck5 wants to merge 10 commits intopygame-community:mainfrom
Open
Conversation
Supports SDL3 mixer constructs in close to their native form. Right now it's as a private module that only builds in SDL3. This is a stepping stone towards overall mixer support for SDL3 builds, with a compatibility layer exposing old API on top of the new, along with giving folks the new powerful stuff directly.
Starbuck5
added
the
New API
Contributor
📝 WalkthroughWalkthroughAdds SDL3 audio and SDL3_mixer support: new C extension modules (_base_audio, _sdl3_mixer_c), Python wrappers and stubs, build/meson/CI updates to conditionally build/install SDL3 mixer, and mypy/stub install packaging tweaks. Changes
Sequence Diagram(s)sequenceDiagram
actor App as "Python App"
participant PyFacade as "src_py/_audio.py\nsrc_py/_sdl3_mixer.py"
participant CExt as "_base_audio / _sdl3_mixer_c"
participant SDL as "SDL / SDL_mixer"
rect rgba(100,150,240,0.5)
App->>PyFacade: init()/open device/play/stream
PyFacade->>CExt: call init/open/create/bind/play APIs
CExt->>SDL: SDL_OpenAudioDevice / SDL audio/mixer calls
SDL-->>CExt: results, audio data, errors
CExt-->>PyFacade: Python objects / results / raise errors
PyFacade-->>App: high-level objects / exceptions
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@buildconfig/stubs/pygame/_sdl3_mixer.pyi`:
- Around line 13-14: The implementations of ms_to_frames and frames_to_ms in
src_py/_sdl3_mixer.py use true division (/) but the stub and runtime signatures
declare -> int; change the division operators in the functions
ms_to_frames(sample_rate, ms) and frames_to_ms(sample_rate, frames) to integer
(floor) division (//) so they return int and remain consistent with the .pyi
stub, and keep the stub signatures as -> int.
In `@meson.build`:
- Around line 150-153: The SDL_mixer version variable sdl_mixer_ver is set to a
pre-release (3.1.2) for SDL3; change sdl_mixer_ver to a stable release (or match
the stability strategy of other SDL3 libs) by replacing the pre-release value in
the conditional that sets sdl_mixer_ver (the ternary expression using sdl_api)
with the first stable SDL_mixer 3.x version (or change both SDL3 dependency
assignments to use pre-release if intentionally aligning), ensuring
sdl_mixer_ver aligns with sdl_ver/sdl_image_ver/sdl_ttf_ver stability.
In `@src_c/_base_audio.c`:
- Around line 196-262: These METH_FASTCALL functions read args[] without
validating nargs, which can lead to OOB reads; add explicit argument-count
guards at the top of each function to check nargs before dereferencing args: for
pg_audio_set_audio_device_gain require nargs == 2, for
pg_audio_open_audio_device allow nargs == 1 or nargs == 4, and require exact
counts for create_audio_stream (6), bind_audio_stream (2), get_audio_stream_data
(2), put_audio_stream_data (2), set_audio_stream_format (3),
set_audio_stream_gain (2), and set_audio_stream_frequency_ratio (2); if the
check fails, set a Python error (e.g. PyErr_SetString(PyExc_TypeError, "bad
call") or PyErr_BadCall()) and return NULL so no args[] access happens.
In `@src_c/_sdl3_mixer_c.c`:
- Around line 90-107: The PyObject returned by PyObject_GetAttrString (e.g., the
local audio_type in pg_mixer_obj_play_audio) is a new reference and must be
Py_DECREF'd on every return path; modify pg_mixer_obj_play_audio to store the
attribute result, use it in PyArg_ParseTupleAndKeywords, and call
Py_DECREF(audio_type) before any early return (NULL on parse failure or on
MIX_PlayAudio failure) and before the successful Py_RETURN_NONE path; apply the
same pattern to the other functions with PyObject_GetAttrString occurrences (the
blocks around lines 396-418, 474-493, 514-536, 810-828) so each temporary is
released on all code paths.
- Around line 109-149: The PyArg_ParseTupleAndKeywords format strings are wrong
and must be fixed to match the actual C parameter types and counts: update
pg_mixer_obj_play_tag to use "s|LLLLLL" (one required tag 's' and six optional
int64_t 'L' for loops, max_ms, start_ms, loop_start_ms, fadein_ms,
append_silence_ms) and ensure the keywords[] order matches the arg list; update
pg_mixer_obj_set_tag_gain to use "s f" as "sf" (or "s|f" if gain is optional) so
the tag (char*) and gain (double/float) are parsed correctly; update
pg_mixer_obj_stop_all_tracks to use "L" (int64_t) instead of "s" for the
fade_out_ms argument; also verify the argument pointers passed to
PyArg_ParseTupleAndKeywords match those format specifiers and the keywords
arrays for each function.
- Around line 314-319: The deallocators must untrack GC, clear owned Python
references, and free memory; update pg_mixer_obj_dealloc and the other two GC
deallocators to call PyObject_GC_UnTrack((PyObject*)self) before releasing
references, call MIX_DestroyMixer(self->mixer) and set self->mixer = NULL (as
already present), clear any PyObject* fields with Py_CLEAR (specifically add
Py_CLEAR(self->source_obj) inside pg_track_obj_dealloc), and finally free the
object with Py_TYPE(self)->tp_free((PyObject*)self); ensure this pattern
(untrack, Py_CLEAR for all owned PyObject* fields, native cleanup, tp_free) is
applied to all three deallocator functions.
In `@src_py/_sdl3_mixer.py`:
- Around line 10-27: The functions ms_to_frames and frames_to_ms currently use
floating-point division and return floats despite typing -> int; change the
arithmetic to integer math so they return ints: in ms_to_frames use integer
multiplication/division (e.g., compute ms * sample_rate // 1000) and in
frames_to_ms compute frames * 1000 // sample_rate (both using // or ordering to
avoid intermediate floats), keeping the existing input validation and return
type.
🧹 Nitpick comments (1)
src_c/meson.build (1)
435-463: Clarify the SDL3 TODO now that an SDL3 mixer path exists.The TODO suggests SDL3 isn't supported, but this block now builds _sdl3_mixer_c. Consider rewording to point at remaining gaps (e.g., compatibility layer) to avoid confusion.
💡 Possible rewording
-# TODO: support SDL3 +# TODO: SDL3 mixer compatibility layer (pygame.mixer API) still pending
Member
Author
|
@coderabbitai Please re-review! |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src_c/_sdl3_mixer_c.c`:
- Around line 846-855: In pg_track_obj_set_freq_ratio, check for a Python-level
error after calling PyFloat_AsDouble(value) by calling PyErr_Occurred(); if an
error occurred, return -1 immediately and do not call
MIX_SetTrackFrequencyRatio. Update the function (pg_track_obj_set_freq_ratio) to
detect PyFloat_AsDouble failures and propagate the Python exception instead of
using the returned -1.0 as a valid ratio, ensuring MIX_SetTrackFrequencyRatio is
only called with a successfully converted double.
- Around line 134-150: SDL_CreateProperties allocates a properties object that
is never freed here; update the play_tag implementation to call
SDL_DestroyProperties(options) after use: ensure you destroy options after
calling MIX_PlayTag(self->mixer, tag, options) in all paths (both success and
failure) and also destroy it before returning on the early error path when
pg_populate_play_props returns false; reference the symbols
SDL_CreateProperties, SDL_DestroyProperties, pg_populate_play_props, and
MIX_PlayTag and make sure no return happens before
SDL_DestroyProperties(options) has been called.
- Around line 778-789: In pg_track_obj_dealloc, ensure GC untracking occurs
before clearing references and include clearing of source_obj: call
PyObject_GC_UnTrack(self) immediately after any necessary non-Python teardown
(e.g., after MIX_DestroyTrack(self->track) and setting self->track = NULL), then
Py_CLEAR(self->source_obj) and Py_CLEAR(self->mixer_obj) to drop both Python
references safely; afterwards proceed with retrieving the free function via
PyType_GetSlot(Py_TYPE(self), Py_tp_free), call free(self) and
Py_DECREF(Py_TYPE(self)) as currently done.
- Around line 1040-1054: The properties object created as SDL_CreateProperties()
in Track.play (variable options) is not destroyed on all paths; update
Track.play to call SDL_DestroyProperties(options) after use (both on success and
error paths) similar to Mixer.play_tag: after calling
pg_populate_play_props(...) and MIX_PlayTrack(self->track, options) ensure
SDL_DestroyProperties(options) is invoked before returning (and also before
raising the SDL error when MIX_PlayTrack fails) so the properties are always
freed.
- Around line 582-598: The PGAudioObject constructor currently leaks `self` when
MIX_CreateSineWaveAudio fails and incorrectly increments `self`'s refcount;
update the error path to Py_DECREF(self) before returning NULL when
`MIX_CreateSineWaveAudio` returns NULL, and remove the spurious Py_INCREF(self)
before returning the new object; locate the allocation/free logic around
PGAudioObject *self = (PGAudioObject *)cls->tp_alloc(cls, 0) and the
MIX_CreateSineWaveAudio call to apply these changes.
- Around line 539-553: PGAudioObject allocated by cls->tp_alloc (self) is leaked
if MIX_LoadRawAudio fails and is over-retained by the extraneous
Py_INCREF(self); fix by, on the MIX_LoadRawAudio(NULL) error path,
Py_DECREF(self) (and any temporary objects like bytes if still owned) before
returning the RAISE(pgExc_SDLError, SDL_GetError()), and remove the unnecessary
Py_INCREF(self) before returning since tp_alloc already returns refcount 1;
update the block around MIX_LoadRawAudio, self->audio assignment, and the return
to reflect these changes.
In `@src_py/_sdl3_mixer.py`:
- Around line 69-80: The TypeError message in Audio.from_raw incorrectly says
"Track 'spec'..." — update the message in the from_raw classmethod to reference
"Audio 'spec' argument" (or simply "Audio spec") when spec is not an instance of
audio.AudioSpec; modify the raise TypeError call inside from_raw to use the
corrected string while keeping the existing type interpolation with {type(spec)}
to preserve detail.
🧹 Nitpick comments (4)
.github/workflows/build-sdl3.yml (1)
116-125: Pin SDL3_mixer to a specific release tag for CI reproducibility and stability.Cloning the default branch makes CI non-reproducible and susceptible to upstream breakages. Pin to
prerelease-3.1.2(the latest SDL3-compatible SDL_mixer release) and use a shallow clone to reduce build time and bandwidth:🔧 Proposed change
- git clone https://github.com/libsdl-org/SDL_mixer + git clone --depth 1 --branch prerelease-3.1.2 https://github.com/libsdl-org/SDL_mixerNote: The same improvement should be applied to SDL3, SDL3_image, and SDL3_ttf builds (lines 86, 97, 108).
src_c/_sdl3_mixer_c.c (2)
1460-1483: Consider releasing type references on error paths inexec_mixer.If
PyModule_AddObjectReforPyObject_SetAttrStringfails partway through, the locally held type references (mixer_type,audio_type,track_type) may leak. While Python cleans up the module on failure, the local references created byPyType_FromModuleAndSpecwon't be automatically decremented.This is a minor edge case during module initialization failure.
1004-1008: Remove duplicate macro definition.
SET_NUM_PROPERTY_IFNOTDEFAULT_ANDFLAGis already defined at Lines 37-41. This duplicate at Lines 1004-1008 is unnecessary.🔧 Proposed fix
-#define SET_NUM_PROPERTY_IFNOTDEFAULT_ANDFLAG(props, property, value, \ - default, success) \ - if (value != default) { \ - success &= SDL_SetNumberProperty(props, property, value); \ - } - static PyObject * pg_track_obj_play(PGTrackObject *self, PyObject *args, PyObject *kwargs)buildconfig/stubs/pygame/_sdl3_mixer.pyi (1)
1-7: Unused imports can be cleaned up or marked with# noqa.Lines 2-3 import
Callable,Any,Type,TypedDict,TypeVarwhich are only used in commented-out callback type definitions. Consider removing them or adding a comment indicating they're reserved for future use.
Member
Author
|
@coderabbitai Bring it on, is there anything else that's off? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 10585 🏁 Script executed: Length of output: 11845 🏁 Script executed: Length of output: 6749 🏁 Script executed: Length of output: 5353 🏁 Script executed: Length of output: 1903 🏁 Script executed: Length of output: 3241 🏁 Script executed: Length of output: 2495 🏁 Script executed: Length of output: 1534
🔴 Critical Gaps1. No Test Coverage
2. Missing Documentation
3. Quit Safety Concern
🟡 Polish Needed4. Missing
5. Error Message Clarity
✅ What Looks Good
Since this is a draft PR and part of a larger SDL3 migration effort, these gaps are expected. The foundation is solid - just needs tests, docs, and some cleanup before it's ready to merge. 🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src_c/_sdl3_mixer_c.c`:
- Around line 1428-1434: MIX_GetAudioDecoder can return NULL so guard the call
before passing it to PyUnicode_FromString: in the loop call const char *name =
MIX_GetAudioDecoder(i); if (name == NULL) set a Python exception (e.g.,
PyErr_Format(PyExc_RuntimeError, "MIX_GetAudioDecoder returned NULL for index
%d", i)), Py_DECREF(decoders) and return NULL; otherwise call
PyUnicode_FromString(name) and proceed with PyList_SetItem as before. This
prevents calling PyUnicode_FromString(NULL) and ensures proper error cleanup of
decoders.
- Around line 934-945: The code calls
PyObject_GetAttrString(audiostream_or_none, "_state") into
PGAudioStreamStateObject *as_state and on NULL currently calls
RAISE(pgExc_SDLError,...), which masks the original AttributeError; change the
NULL handling so that if PyObject_GetAttrString returns NULL you simply return
NULL (allowing the existing AttributeError to propagate) instead of raising
pgExc_SDLError—locate the block using PyObject_GetAttrString,
PGAudioStreamStateObject *as_state, and the subsequent RAISE call and replace
the error path accordingly.
- Around line 1451-1493: The type objects created in exec_mixer (mixer_type,
audio_type, track_type) are new references from PyType_FromModuleAndSpec and are
not DECREF'd after adding to the module, leaking references and leaving error
paths uncleaned; after each successful PyModule_AddObjectRef call DECREF the
local variable (call Py_DECREF on mixer_type, audio_type, track_type as
appropriate) and on any error path ensure previously-created types are DECREF'd
before returning (e.g., if adding track_type fails DECREF audio_type and
mixer_type), so all local new references are released while the module retains
its references.
- Around line 277-316: In pg_mixer_obj_init, guard against non-tuple or short
tuple spec_obj: before calling PyTuple_GetItem/PyLong_AsInt, use
PyTuple_Check(spec_obj) and verify PyTuple_Size(spec_obj) >= 3 (or call
PyArg_ParseTuple on spec_obj), then retrieve each item and check each
PyTuple_GetItem return for NULL before passing to PyLong_AsInt; if any check
fails, propagate/return -1 after ensuring an appropriate Python error is set so
spec_p is only set when all fields were successfully parsed.
🧹 Nitpick comments (2)
src_py/_sdl3_mixer.py (1)
109-118: Simplifyset_audiostream— both valid branches call the same function.The
isinstanceandis Nonebranches both delegate to the same C method with the same argument. You can collapse them:Suggested simplification
def set_audiostream(self, audiostream: audio.AudioStream | None) -> None: - if isinstance(audiostream, audio.AudioStream): - _sdl3_mixer_c.Track.set_audiostream(self, audiostream) - elif audiostream is None: + if isinstance(audiostream, audio.AudioStream) or audiostream is None: _sdl3_mixer_c.Track.set_audiostream(self, audiostream) else: raise TypeError( "audiostream argument must be an AudioStream or None, " f"received {type(audiostream)}" )src_c/_sdl3_mixer_c.c (1)
318-328:PyObject_GC_UnTrackshould be called beforeMIX_DestroyMixer.The CPython convention for GC-tracked types is to untrack the object as the very first thing in dealloc, before any other cleanup, to prevent the GC from visiting a partially-destroyed object. Currently
MIX_DestroyMixerruns first. The same applies topg_audio_obj_dealloc(Line 446).While harmless for
PGMixerObject(no Python references to visit), it's best practice to be consistent—especially sincepg_track_obj_dealloc(Line 782) already has GC_UnTrack beforePy_CLEAR.Proposed fix
static void pg_mixer_obj_dealloc(PGMixerObject *self) { + PyObject_GC_UnTrack(self); MIX_DestroyMixer(self->mixer); self->mixer = NULL; - PyObject_GC_UnTrack(self); PyTypeObject *tp = Py_TYPE(self);
Comment on lines
+277
to
+316
| static int | ||
| pg_mixer_obj_init(PGMixerObject *self, PyObject *args, PyObject *kwargs) | ||
| { | ||
| PyObject *device_obj; | ||
| PyObject *spec_obj = Py_None; | ||
|
|
||
| char *keywords[] = {"device", "spec", NULL}; | ||
| if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|O", keywords, | ||
| &device_obj, &spec_obj)) { | ||
| return -1; | ||
| } | ||
|
|
||
| SDL_AudioSpec *spec_p = NULL; | ||
| SDL_AudioSpec spec; | ||
| // if the passed in spec obj is not None, we assume it is a tuple | ||
| // of elements created by the Python layer for us. | ||
| if (spec_obj != Py_None) { | ||
| spec.format = PyLong_AsInt(PyTuple_GetItem(spec_obj, 0)); | ||
| spec.channels = PyLong_AsInt(PyTuple_GetItem(spec_obj, 1)); | ||
| spec.freq = PyLong_AsInt(PyTuple_GetItem(spec_obj, 2)); | ||
|
|
||
| // Check that they all succeeded | ||
| if (spec.format == -1 || spec.channels == -1 || spec.freq == -1) { | ||
| if (PyErr_Occurred()) { | ||
| return -1; | ||
| } | ||
| } | ||
|
|
||
| spec_p = &spec; | ||
| } | ||
|
|
||
| self->mixer = MIX_CreateMixerDevice( | ||
| ((PGAudioDeviceStateObject *)device_obj)->devid, spec_p); | ||
| if (self->mixer == NULL) { | ||
| PyErr_SetString(pgExc_SDLError, SDL_GetError()); | ||
| return -1; | ||
| } | ||
|
|
||
| return 0; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine the actual code
file src_c/_sdl3_mixer_c.c && wc -l src_c/_sdl3_mixer_c.cRepository: pygame-community/pygame-ce
Length of output: 114
🏁 Script executed:
# Read the specific lines mentioned (277-316)
sed -n '277,316p' src_c/_sdl3_mixer_c.c | cat -nRepository: pygame-community/pygame-ce
Length of output: 1588
🏁 Script executed:
# Check the actual line numbers where PyTuple_GetItem calls occur in this function
sed -n '277,316p' src_c/_sdl3_mixer_c.c | grep -n "PyTuple_GetItem"Repository: pygame-community/pygame-ce
Length of output: 276
🏁 Script executed:
# Search for the type definition and TPFLAGS configuration
rg -n "PGMixerObject\|TPFLAGS" src_c/_sdl3_mixer_c.c | head -30Repository: pygame-community/pygame-ce
Length of output: 52
🏁 Script executed:
# Look for the type definition to see if BASETYPE flag is set
rg -B5 -A10 "PyTypeObject.*Mixer\|\.tp_base" src_c/_sdl3_mixer_c.c | head -40Repository: pygame-community/pygame-ce
Length of output: 52
🏁 Script executed:
# Search more broadly for the type definition
rg -n "PGMixerType\|PyTypeObject.*pg_mixer" src_c/_sdl3_mixer_c.cRepository: pygame-community/pygame-ce
Length of output: 52
🏁 Script executed:
# Look for the full type struct definition
rg -B2 -A15 "\.tp_init.*pg_mixer_obj_init" src_c/_sdl3_mixer_c.cRepository: pygame-community/pygame-ce
Length of output: 52
🏁 Script executed:
# Check if there are any other files that might define the type flags
fd "_sdl3_mixer" src_c/Repository: pygame-community/pygame-ce
Length of output: 91
🌐 Web query:
PyTuple_GetItem NULL dereference PyLong_AsInt behavior CPython
💡 Result:
PyTuple_GetItem: when/why you can get NULL
PyTuple_GetItem(p, pos) returns a borrowed reference, but returns NULL and sets IndexError if pos is negative or out of bounds. If you pass that NULL onward, you risk a crash (C-API functions generally assume non-NULL inputs). [1]
PyLong_AsInt: not a public CPython API (and error handling semantics)
There is no public PyLong_AsInt() in the documented C-API; what exists in CPython headers is the private function _PyLong_AsInt(PyObject *). [2]
For integer-conversion APIs in general, CPython documents that most PyLong_As* functions return (type)-1 on error, so you must use PyErr_Occurred() (and/or check exceptions) to disambiguate error vs a legitimate -1. [3]
Safe pattern (avoid NULL deref + handle overflow/errors)
PyObject *obj = PyTuple_GetItem(args, 0);
if (obj == NULL) return NULL; // IndexError already set by CPython
long v = PyLong_AsLong(obj);
if (v == -1 && PyErr_Occurred()) return NULL;
if (v < INT_MIN || v > INT_MAX) {
PyErr_SetString(PyExc_OverflowError, "int out of range");
return NULL;
}
int i = (int)v;If you meant to parse an int argument, prefer PyArg_ParseTuple(args, "i", &i) which handles type/range checking for you.
Sources: [1] PyTuple_GetItem docs (borrowed ref, returns NULL + IndexError) • [2] _PyLong_AsInt declared as private in CPython Include/cpython/longobject.h • [3] PyLong_As* error/PyErr_Occurred() guidance in Integer Objects docs.
PyTuple_GetItem(NULL-result) → crash if spec_obj is not a proper tuple.
Lines 294–296 call PyTuple_GetItem without checking for NULL returns, then pass those NULL results directly to PyLong_AsInt. According to Python C-API specifications, PyTuple_GetItem returns NULL and sets IndexError when the index is out of bounds or the object is not a tuple, and C-API functions assume non-NULL inputs. If spec_obj is not a proper tuple or has fewer than 3 items, the immediate call to PyLong_AsInt(NULL) risks a crash before the error check on lines 23–26 can execute.
While the Python layer wrapper validates this correctly, the C type has TPFLAGS_BASETYPE enabled, allowing direct C-level construction or subclassing to bypass Python validation.
Add a PyTuple_Check(spec_obj) guard and verify tuple size, or use PyArg_ParseTuple to safely parse the spec tuple.
🤖 Prompt for AI Agents
In `@src_c/_sdl3_mixer_c.c` around lines 277 - 316, In pg_mixer_obj_init, guard
against non-tuple or short tuple spec_obj: before calling
PyTuple_GetItem/PyLong_AsInt, use PyTuple_Check(spec_obj) and verify
PyTuple_Size(spec_obj) >= 3 (or call PyArg_ParseTuple on spec_obj), then
retrieve each item and check each PyTuple_GetItem return for NULL before passing
to PyLong_AsInt; if any check fails, propagate/return -1 after ensuring an
appropriate Python error is set so spec_p is only set when all fields were
successfully parsed.
Comment on lines
+1428
to
+1434
| for (int i = 0; i < num_decoders; i++) { | ||
| PyObject *decoder = PyUnicode_FromString(MIX_GetAudioDecoder(i)); | ||
| if (decoder == NULL || PyList_SetItem(decoders, i, decoder)) { | ||
| Py_DECREF(decoders); | ||
| return NULL; // error already set | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
MIX_GetAudioDecoder could return NULL, which would crash PyUnicode_FromString.
If MIX_GetAudioDecoder(i) returns NULL (e.g., index out-of-range race, or SDL internal error), PyUnicode_FromString(NULL) is undefined behavior.
Proposed defensive fix
for (int i = 0; i < num_decoders; i++) {
- PyObject *decoder = PyUnicode_FromString(MIX_GetAudioDecoder(i));
+ const char *name = MIX_GetAudioDecoder(i);
+ if (name == NULL) {
+ Py_DECREF(decoders);
+ return RAISE(pgExc_SDLError, "Failed to get decoder name");
+ }
+ PyObject *decoder = PyUnicode_FromString(name);
if (decoder == NULL || PyList_SetItem(decoders, i, decoder)) {🤖 Prompt for AI Agents
In `@src_c/_sdl3_mixer_c.c` around lines 1428 - 1434, MIX_GetAudioDecoder can
return NULL so guard the call before passing it to PyUnicode_FromString: in the
loop call const char *name = MIX_GetAudioDecoder(i); if (name == NULL) set a
Python exception (e.g., PyErr_Format(PyExc_RuntimeError, "MIX_GetAudioDecoder
returned NULL for index %d", i)), Py_DECREF(decoders) and return NULL; otherwise
call PyUnicode_FromString(name) and proceed with PyList_SetItem as before. This
prevents calling PyUnicode_FromString(NULL) and ensures proper error cleanup of
decoders.
Comment on lines
+1451
to
+1493
| int | ||
| exec_mixer(PyObject *module) | ||
| { | ||
| /*imported needed apis*/ | ||
| import_pygame_base(); | ||
| if (PyErr_Occurred()) { | ||
| return -1; | ||
| } | ||
| import_pygame_rwobject(); | ||
| if (PyErr_Occurred()) { | ||
| return -1; | ||
| } | ||
|
|
||
| PyObject *mixer_type = PyType_FromModuleAndSpec(module, &mixer_spec, NULL); | ||
| if (PyModule_AddObjectRef(module, "Mixer", mixer_type) < 0) { | ||
| return -1; | ||
| } | ||
|
|
||
| PyObject *audio_type = PyType_FromModuleAndSpec(module, &audio_spec, NULL); | ||
| if (PyModule_AddObjectRef(module, "Audio", audio_type) < 0) { | ||
| return -1; | ||
| } | ||
|
|
||
| PyObject *track_type = PyType_FromModuleAndSpec(module, &track_spec, NULL); | ||
| if (PyModule_AddObjectRef(module, "Track", track_type) < 0) { | ||
| return -1; | ||
| } | ||
|
|
||
| if (PyObject_SetAttrString(mixer_type, "_audio_type", audio_type) < 0) { | ||
| return -1; | ||
| } | ||
| if (PyObject_SetAttrString(track_type, "_audio_type", audio_type) < 0) { | ||
| return -1; | ||
| } | ||
| if (PyObject_SetAttrString(audio_type, "_mixer_type", mixer_type) < 0) { | ||
| return -1; | ||
| } | ||
|
|
||
| _mixer_state *state = GET_STATE(module); | ||
| state->mixer_initialized = false; | ||
|
|
||
| return 0; | ||
| } |
Contributor
There was a problem hiding this comment.
Reference leak: type objects created in exec_mixer are never Py_DECREF'd.
PyType_FromModuleAndSpec returns a new reference. PyModule_AddObjectRef increments the refcount (it does not steal). The local references mixer_type, audio_type, and track_type are never released, leaking one reference each per module init. Error paths also don't clean up already-created types.
Proposed fix
+ int ret = -1;
+ PyObject *mixer_type = NULL, *audio_type = NULL, *track_type = NULL;
+
- PyObject *mixer_type = PyType_FromModuleAndSpec(module, &mixer_spec, NULL);
+ mixer_type = PyType_FromModuleAndSpec(module, &mixer_spec, NULL);
+ if (mixer_type == NULL) {
+ goto error;
+ }
if (PyModule_AddObjectRef(module, "Mixer", mixer_type) < 0) {
- return -1;
+ goto error;
}
- PyObject *audio_type = PyType_FromModuleAndSpec(module, &audio_spec, NULL);
+ audio_type = PyType_FromModuleAndSpec(module, &audio_spec, NULL);
+ if (audio_type == NULL) {
+ goto error;
+ }
if (PyModule_AddObjectRef(module, "Audio", audio_type) < 0) {
- return -1;
+ goto error;
}
- PyObject *track_type = PyType_FromModuleAndSpec(module, &track_spec, NULL);
+ track_type = PyType_FromModuleAndSpec(module, &track_spec, NULL);
+ if (track_type == NULL) {
+ goto error;
+ }
if (PyModule_AddObjectRef(module, "Track", track_type) < 0) {
- return -1;
+ goto error;
}
if (PyObject_SetAttrString(mixer_type, "_audio_type", audio_type) < 0) {
- return -1;
+ goto error;
}
if (PyObject_SetAttrString(track_type, "_audio_type", audio_type) < 0) {
- return -1;
+ goto error;
}
if (PyObject_SetAttrString(audio_type, "_mixer_type", mixer_type) < 0) {
- return -1;
+ goto error;
}
_mixer_state *state = GET_STATE(module);
state->mixer_initialized = false;
- return 0;
+ ret = 0;
+
+error:
+ Py_XDECREF(mixer_type);
+ Py_XDECREF(audio_type);
+ Py_XDECREF(track_type);
+ return ret;
}🤖 Prompt for AI Agents
In `@src_c/_sdl3_mixer_c.c` around lines 1451 - 1493, The type objects created in
exec_mixer (mixer_type, audio_type, track_type) are new references from
PyType_FromModuleAndSpec and are not DECREF'd after adding to the module,
leaking references and leaving error paths uncleaned; after each successful
PyModule_AddObjectRef call DECREF the local variable (call Py_DECREF on
mixer_type, audio_type, track_type as appropriate) and on any error path ensure
previously-created types are DECREF'd before returning (e.g., if adding
track_type fails DECREF audio_type and mixer_type), so all local new references
are released while the module retains its references.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supports SDL3 mixer constructs in close to their native form. Right now it's as a private module that only builds in SDL3. This is a stepping stone towards overall mixer support for SDL3 builds, with a compatibility layer exposing old API on top of the new, along with giving folks the new powerful stuff directly.
Second PR after #3657 in the #3581 mega issue.
Branch comes from #3657, so changes from there will be shown here as well.